Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

graph: backend: passes: verbose log enhancement #2320

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rongzha1
Copy link
Contributor

verbose log enhancement for compile passes

@rongzha1 rongzha1 added the component:graph-api Codeowner: @oneapi-src/onednn-graph label Dec 27, 2024
@rongzha1 rongzha1 requested a review from a team as a code owner December 27, 2024 06:21
@rongzha1
Copy link
Contributor Author

make test
enable benchdnn_nightly
disable benchdnn_all
enable benchdnn_graph

@@ -45,12 +49,15 @@ status_t compile_ops(std::shared_ptr<subgraph_t> &sg) {
= op_schema_registry_t::get_op_schema(op->get_kind());
if (!opm) {
assertm(false, "no schema for current op");
Copy link
Contributor

@wzt1997 wzt1997 Dec 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have to keep the assertm after enriching the verbose? If not, maybe we can fuse the if condition check into the macro.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assertm will abort in debug mode, verbose check only print one log. I think they have different debug scenarios. So I just keep the asserm.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also feel the assertions are less useful now after verbose logs are added.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just removed assertm and wrapped the condition check into VCHECK. Thank you all.

#define VCHECK_MEMORY_PLANNING(cond, status, msg, ...) \
VCONDCHECK(graph, create, check, memory_planning, (cond), status, msg, \
##__VA_ARGS__);

namespace dnnl {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

random place: do we also need to add check here on L351

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added now. thanks

per_channel_broadcast)) {
return status::invalid_shape;
}
VCHECK_INSERT_OPS(prelu_doable(ltw(src_lt).vdims(), ltw(wei_lt).vdims(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest to put prelu_doable() into a separate line and pass its return value to VCHECK_INSERT_OPS. It will make the code easier to read and debug.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed as suggested. please take a look again. thanks

@@ -45,12 +49,15 @@ status_t compile_ops(std::shared_ptr<subgraph_t> &sg) {
= op_schema_registry_t::get_op_schema(op->get_kind());
if (!opm) {
assertm(false, "no schema for current op");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also feel the assertions are less useful now after verbose logs are added.

@rongzha1 rongzha1 force-pushed the rzhang/verbose_passes branch from bb374d9 to 157df77 Compare January 3, 2025 07:39
@rongzha1
Copy link
Contributor Author

rongzha1 commented Jan 3, 2025

make test
enable benchdnn_nightly
disable benchdnn_all
enable benchdnn_graph

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:graph-api Codeowner: @oneapi-src/onednn-graph
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants